There's a helper type that can encapsulate this logic: otap_df_pdata::arrays::StringArrayAccessor

Instead of having a method defined here specially for this, below in the create_inserted_batch you could do

let key_col = current_batch
            .column_by_name(consts::ATTRIBUTE_KEY)
            .map(StringArrayAccessor::try_new)
            .transpose()?;

Then you could probably return an error if the key_col ends up being None.

This type provides the str_at method which returns an Option<&str>

Instead of using the regular arrow builders and having to write these match statements to append values to arrays that could be dict/native type, we have a set of helper types that can encapsulate this logic.

For example, StringArrayBuilder:

This type also exposes a append_str_n method which can be used to append the same string multiple times. This is usually faster than appending the values one at a time. So an optimization we could make here, if we're appending the same key multiple times, would be to use this method.

There are similar builders for the other types we're inserting below as values (int, double, bool)

I'm wondering if there are any ways we could optimize building up the set of attributes we're going to insert.

For example, using a BTreeMap here, and a BTreeSet below for unique_parents means that we'll be hashing every parent_id multiple times. It might be faster to use a RoaringBitmap for the unique_parent_ids, and maybe we could have a RoaringBitmap for each insert entry corresponding to whether the row w/ some parent_id contains the attribute.

The parent_ids won't always be u16. For example, for metrics datapoint attributes and attributes for span link and span event, the parent ID type can be u32. Also, for u32 IDs, the parent_id_arr can be dictionary encoded (e.g. it's not always a PrimitiveArray).

To handle this, we might need to make the function generic over T where:

    T: ParentId,
    <T as ParentId>::ArrayType: ArrowPrimitiveType,

(You'll see we do something similar in this file for materialize_parent_id_for_attributes).

Then we can get the parent_ids as:

To ensure we hande u32 parent IDs correctly, it probably also makes sense to add a test for this somewhere.

There's a case that the logic here might not handle correctly, which is if we're inserting a type of attribute and the original schema did not previously contain any attributes of this type.

All the ATTRIBUTE_* columns are optional in OTAP. For example, if some attribute RecordBatch had no values of type int, the ATTRIBUTE_INT column would be omitted. If we encountered such a record batch and we were inserting an integer attribute, it would not be included in the original batch with this logic.

We should add a test for this and handle it. We might need to write some custom batch concatenation logic for this, rather than relying on arrows concat_batches` compute kernel, or make some modifications to the original batch before we invoke this function

I think this has been fixed. The necessary metadata is added to schema in extend_schema_for_inserts before the insertion.

I mentioned in https://github.com/open-telemetry/otel-arrow/pull/1737/files#r2704902249 that we might need to be careful about how we invoke this concat_batches function due to the case where we're inserting a column that was not contained in the original batch.

There's another case we probably need to handle as well which is, if the column is dictionary encoded and inserting the new value would cause the dictionary to overflow, then we need to either expand the key type (e.g. convert from a Dict to a Dict) or convert from Dict to non dict encoded array.

FYI in #1824 we made a change so we always materialize the parent_ids. This was the most expedient way to fix a subtle bug (#996)